Add validation rule that operation types exist#955
Conversation
Right now the spec says that, for example, if the schema does not define a mutation root type, then the schema does not support mutations. But there's no validation rule for it, which means that many parsers (including graphql-js) treat a mutation as valid against such a schema. (Indeed, many end up considering *any* mutation as valid, since they don't know what type to validate the root selection set against.) This commit adds a validation rule to make the schema text explicit. Slated for discussion at the June 2 working group meeting. See also graphql/graphql-js#3592.
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Excellent! I'm not convinced we need the changes to section 3, but the section 5 changes look solid short of some bikeshedding over inserting "All Operation Definitions" versus elevating the "Operation Type Existence" header. |
|
Ok! I don't feel strongly about mentioning it in section 3. BTW, the other thing to discuss is whether the rule that a schema must have a query and that the root types must be object types should have explicit validation language as well. It seems like the schema side is a lot less consistent about that, so I wasn't sure. We can discuss tomorrow. |
|
The validation section (section 5) is about validation of requests; the schema has its own validation rules in the relevant places, for example the rules about root operation types can be found here: |
Turns out neither we (nor anyone else) were validating this, because the
spec didn't say explicitly to validate it. So you could do
`mutation { bogus }` even if the schema has no mutation types, or worse,
any syntactically valid query if the schema is totally empty.
In graphql/graphql-spec#955 I'm adding it to the spec, and in
graphql/graphql-js#3592 someone else is adding it to graphql-js. So we
should too, once those land! At that point we should also likely
reimport the relevant tests, instead of the ones I wrote here, but I
figured I'd put the PR up now so folks can review the non-test parts if
you like.
Fixes vektah#221.
Turns out neither we (nor anyone else) were validating this, because the
spec didn't say explicitly to validate it. So you could do
`mutation { bogus }` even if the schema has no mutation types, or worse,
any syntactically valid query if the schema is totally empty.
In graphql/graphql-spec#955 I'm adding it to the spec, and in
graphql/graphql-js#3592 someone else is adding it to graphql-js. So we
should too, once those land! At that point we should also likely
reimport the relevant tests, instead of the ones I wrote here, but I
figured I'd put the PR up now so folks can review the non-test parts if
you like.
Fixes #221.
It seems to me that validation of a document is different than validation of a schema. Section 5 validates "if a request is syntactically correct" and " is unambiguous and mistake‐free in the context of a given GraphQL schema". So I don't believe section 5 is intended to cover validation of a schema, and I would not add any schema validation language there. Rather, section 3 "describes the capabilities of a GraphQL server", and already has language stating that a query root type must exist, etc. Perhaps it is worth considering rewriting all of section 3 to include schema validation rules more like the "formal specification" syntax used in section 5. However, this may be more challenging in that a schema is not required to be represented by a SDL. |
|
Yeah, to be clear, we would not add schema validation to section 5, if we wanted to add that it would go in section 3 and there's some precedent for validation rules there. As you say making that validation more formal is probably a bigger project, so I left it out for now; as currently written this is all about document validation. |
benjie
left a comment
benjie
left a comment
There was a problem hiding this comment.
Looking good, couple minor fixes 👍
Co-authored-by: Benjie Gillam <benjie@jemjie.com>
Co-authored-by: Benjie <benjie@jemjie.com>
|
I have:
I think it's good to go now! 🤞 |
Co-authored-by: Shane Krueger <shane@acdmail.com>
Shane32
left a comment
Shane32
left a comment
There was a problem hiding this comment.
Short and to the point. I like it. 👍
|
@yaacovCR has an implementation of this here: |
leebyron
added
📄 Draft (RFC 2)
| - For each operation definition {operation} in the document: | ||
| - Let {rootOperationType} be the _root operation type_ in {schema} | ||
| corresponding to the kind of {operation}. | ||
| - {rootOperationType} must exist. |
There was a problem hiding this comment.
| - {rootOperationType} must exist. | |
| - {rootOperationType} must be defined in {schema}. |
or schema without curlies, I see both in the other algorithms
There was a problem hiding this comment.
We have existing algorithms that follow this pattern, e.g.
graphql-spec/spec/Section 5 -- Validation.md
Lines 664 to 666 in b41339a
benjie
force-pushed
the
operation-type-validation
branch
from
2bc85e1 to
b209bd8
Compare
leebyron
added
🏁 Accepted (RFC 3)
github-project-automation
bot
moved this from Just waiting on approval
to Done
in Benjie's GraphQL tasks
7 participants
Right now the spec says that, for example, if the schema does not define
a mutation root type, then the schema does not support mutations. But
there's no validation rule for it, which means that many parsers
(including graphql-js) treat a mutation as valid against such a schema.
(Indeed, many end up considering any mutation as valid, since they
don't know what type to validate the root selection set against.) This
commit adds a validation rule to make the schema text explicit.
Slated for discussion at the June 2 2022 working group meeting.
Replaces #947. See also graphql/graphql-js#3592.
Update October 2024: this PR has been refreshed including changes proposed in #1098 and Benjie's editorial edits.
Fixes #1097